8382226: [lworld] C2: Fix _copyOf/_copyOfRange intrinsic for flat abstract value class arrays by chhagedorn · Pull Request #2569 · openjdk/valhalla

chhagedorn · 2026-06-19T12:34:36Z

The provided test cases fail when inlining the Array.copyOf/copyOfRange() intrinsics where the source array is flat and from an abstract value class.

The current code checks whether the source array or the destination array klass contain oops by assuming that a flat value class array is always concrete and thus an InlineKlass (i.e. can call inline_klass()). However, we could also have abstract value class arrays that are known to be flat (see test cases). This leads to a cast assertion failure because abstract value classes are represented by an InstanceKlass and not an InlineKlass.

To fix this, I added a simple bailout when detecting an abstract flat value class array. This is a conservative correctness fix and should be revisited again post-Valhalla-integration. We have JDK-8251971 in place for that which should also tackle other issues around the arraycopy intrinsics and also address performance problems.

Thanks,
Christian

I confirm that I make this contribution in accordance with the OpenJDK Interim AI Policy.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed (1 review required, with at least 1 Committer)

Issue

JDK-8382226: [lworld] C2: Fix _copyOf/_copyOfRange intrinsic for flat abstract value class arrays (Bug - P4)

Reviewers

Quan Anh Mai (@merykitty - Committer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/valhalla.git pull/2569/head:pull/2569
$ git checkout pull/2569

Update a local copy of the PR:
$ git checkout pull/2569
$ git pull https://git.openjdk.org/valhalla.git pull/2569/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 2569

View PR using the GUI difftool:
$ git pr show -t 2569

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/valhalla/pull/2569.diff

Using Webrev

Link to Webrev Comment

bridgekeeper · 2026-06-19T12:36:16Z

👋 Welcome back chagedorn! A progress list of the required criteria for merging this PR into lworld will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2026-06-19T12:37:09Z

@chhagedorn This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8382226: [lworld] C2: Fix _copyOf/_copyOfRange intrinsic for flat abstract value class arrays

Reviewed-by: qamai

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 7 new commits pushed to the lworld branch:

dc8b7c5: 8386676: [lworld] Infinite loop when pushing inline types down though phis during IGVN
43153f5: 8387488: [lworld] C2: acmp expansion fails with assert(replace != nullptr) failed: must succeed
285c9fc: 8387564: [lworld] cherry-pick JDK-8387554 ProblemList vmTestbase/nsk/jvmti/unit/functions/Dispose/JvmtiTest/TestDescription.java in virtual thread configs
... and 4 more: https://git.openjdk.org/valhalla/compare/05767f0f84b7d4ea0ef09739a9a2ef0e5aab234f...lworld

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the lworld branch, type /integrate in a new comment.

mlbridge · 2026-06-19T12:46:05Z

Webrevs

merykitty · 2026-06-19T13:04:43Z

    const TypeAryPtr* orig_t = _gvn.type(original)->isa_aryptr();
    const TypeKlassPtr* tklass = _gvn.type(klass_node)->is_klassptr();
-    bool exclude_flat = UseArrayFlattening && bs->array_copy_requires_gc_barriers(true, T_OBJECT, false, false, BarrierSetC2::Parsing) &&
+    const bool is_src_abstract_flat_value_array = orig_t != nullptr && !orig_t->elem()->is_inlinetypeptr() && orig_t->is_flat();


Should we check for !orig_t->is_not_flat() instead?

That's a good idea to cover all the cases where it could be flat. Updated this and the check for tklass as well accordingly.

merykitty · 2026-06-19T14:41:49Z

                        (orig_t == nullptr || (!orig_t->is_not_flat() && (!orig_t->is_flat() || orig_t->elem()->inline_klass()->contains_oops()))) &&
                        // Can dest array be flat and contain oops?
-                        tklass->can_be_inline_array() && (!tklass->is_flat() || tklass->is_aryklassptr()->elem()->is_instklassptr()->instance_klass()->as_inline_klass()->contains_oops());
+                        can_dest_be_value_class_array && (!tklass->is_flat() || tklass->is_aryklassptr()->elem()->is_instklassptr()->instance_klass()->as_inline_klass()->contains_oops())));


This should also check for tklass->is_not_flat() instead.

I agree. I also took another look at the code again and refactored it a bit to make exclude_flat easier to understand. Let me know, what you think.

… read

merykitty · 2026-06-23T15:27:54Z

-                        tklass->can_be_inline_array() && (!tklass->is_flat() || tklass->is_aryklassptr()->elem()->is_instklassptr()->instance_klass()->as_inline_klass()->contains_oops());
+    const TypeAryKlassPtr* dest_klass_t = _gvn.type(klass_node)->is_klassptr()->isa_aryklassptr();
+    const bool can_src_be_abstract_flat_value_class_array = orig_t != nullptr && !orig_t->elem()->is_inlinetypeptr() && !orig_t->is_not_flat();
+    const bool can_dest_be_value_class_array = dest_klass_t != nullptr && dest_klass_t->can_be_inline_array();


What if dest is not an aryklassptr? I think in that case this should also be true, right?

See general comment sent above.

merykitty · 2026-06-23T15:29:49Z

-                        // Can dest array be flat and contain oops?
-                        tklass->can_be_inline_array() && (!tklass->is_flat() || tklass->is_aryklassptr()->elem()->is_instklassptr()->instance_klass()->as_inline_klass()->contains_oops());
+    const TypeAryKlassPtr* dest_klass_t = _gvn.type(klass_node)->is_klassptr()->isa_aryklassptr();
+    const bool can_src_be_abstract_flat_value_class_array = orig_t != nullptr && !orig_t->elem()->is_inlinetypeptr() && !orig_t->is_not_flat();


This name is a little misleading, I think you want to catch the case orig_t == nullptr below, but if the static type is j.l.O, the runtime type can still be an abstract flat array.

That's true. Though, I'm wondering if orig_t could even be a non-array pointer at all. The Arrays.copyOf() takes a U[], so after type erasure, we should know it's an Object[] and thus have an array
pointer. It looks like the original code was just defensive by checking orig_t as well. I removed it and testing passed.

chhagedorn

Sorry for taking some more time to respond here. Thanks for your additional questions @merykitty. I've had another closer look at the code and wrote some more tests and discovered more issues:

When passing in primitive type mirrors like int.class or for void.class to Arrays.copyOf(), there is no klass field and we return null:

valhalla/src/hotspot/share/opto/memnode.cpp

Lines 2834 to 2837 in dc8b7c5

    
           if (!t->is_klass()) { 
        
             // a primitive Class (e.g., int.class) has null for a klass field 
        
             return TypePtr::NULL_PTR; 
        
           }

As a result, we crash here because klass_node returned from load_klass_from_mirror() is top:

valhalla/src/hotspot/share/opto/library_call.cpp

Line 5195 in dc8b7c5

const TypeKlassPtr* tklass = _gvn.type(klass_node)->is_klassptr();

Solution: Bail out if stopped() after load_klass_from_mirror().

When passing in an instance class mirror like A.class we should throw because it's not an array class. However, with the checks in the current code, we only emit a type array guard which does not bail out when having an instance klass:

valhalla/src/hotspot/share/opto/library_call.cpp

Lines 5196 to 5201 in dc8b7c5

    
           bool exclude_flat = UseArrayFlattening && bs->array_copy_requires_gc_barriers(true, T_OBJECT, false, false, BarrierSetC2::Parsing) && 
        
                               // Can src array be flat and contain oops? 
        
                               (orig_t == nullptr || (!orig_t->is_not_flat() && (!orig_t->is_flat() || orig_t->elem()->inline_klass()->contains_oops()))) && 
        
                               // Can dest array be flat and contain oops? 
        
                               tklass->can_be_inline_array() && (!tklass->is_flat() || tklass->is_aryklassptr()->elem()->is_instklassptr()->instance_klass()->as_inline_klass()->contains_oops()); 
        
           Node* not_objArray = exclude_flat ? generate_non_refArray_guard(klass_node, bailout) : generate_typeArray_guard(klass_node, bailout);

We then try to load the refined array klass from an instance klass pointer in load_default_refined_array_klass() where we read from ObjArrayKlass::next_refined_array_klass_offset() which is garbage.

Solution: Emit generate_non_refArray_guard() when the klass pointer is not an array klass.

We need to check UseArrayFlattening and the GC barrier availability after the inserted check for for 2., otherwise, we hit the same problem again when disabling UseArrayFlattening or not requiring GC barriers at all.

I pushed an update with the following changes:

Refactored bailout logic again to a separate method where I check all the cases systematically:
- Check dest klass to be an array klass.
- Check all cases for maybe flat and bail out which also covers j.l.Object.
- Check write barriers.
Added more comments.
Added a lot more tests by using the Template Framework.

Testing this through t1-4 + stress looked good. Since we now bail out in more cases, I might also run some performance testing.

Let me know, what you think.

chhagedorn · 2026-07-01T08:04:43Z

-                        // Can dest array be flat and contain oops?
-                        tklass->can_be_inline_array() && (!tklass->is_flat() || tklass->is_aryklassptr()->elem()->is_instklassptr()->instance_klass()->as_inline_klass()->contains_oops());
+    const TypeAryKlassPtr* dest_klass_t = _gvn.type(klass_node)->is_klassptr()->isa_aryklassptr();
+    const bool can_src_be_abstract_flat_value_class_array = orig_t != nullptr && !orig_t->elem()->is_inlinetypeptr() && !orig_t->is_not_flat();


That's true. Though, I'm wondering if orig_t could even be a non-array pointer at all. The Arrays.copyOf() takes a U[], so after type erasure, we should know it's an Object[] and thus have an array
pointer. It looks like the original code was just defensive by checking orig_t as well. I removed it and testing passed.

chhagedorn · 2026-07-01T08:46:07Z

-                        tklass->can_be_inline_array() && (!tklass->is_flat() || tklass->is_aryklassptr()->elem()->is_instklassptr()->instance_klass()->as_inline_klass()->contains_oops());
+    const TypeAryKlassPtr* dest_klass_t = _gvn.type(klass_node)->is_klassptr()->isa_aryklassptr();
+    const bool can_src_be_abstract_flat_value_class_array = orig_t != nullptr && !orig_t->elem()->is_inlinetypeptr() && !orig_t->is_not_flat();
+    const bool can_dest_be_value_class_array = dest_klass_t != nullptr && dest_klass_t->can_be_inline_array();


See general comment sent above.

merykitty

Thanks for taking further look, it looks much better now.

chhagedorn · 2026-07-01T10:47:08Z

Thanks @merykitty for your careful review!

merykitty · 2026-07-01T11:29:38Z

+      // Arrays.copyOf() uses a generic Class parameter which is erased to the raw type Class. This also allows
+      // passing in primitive class mirrors like int.class which do not have corresponding Klass* pointers.
+      // In these cases, klass_node will be top. Emit a trap to throw in the interpreter in this case.
+      bail_out_from_array_copyOf(bailout);


What if the j.l.Class is not a constant, but the runtime value is int.class, it will return null, right? Which explains the null check below. In that case, is it an inconsistency? Can it be an issue if a j.l.Class is narrowed to the constant int.class during IGVN, then the node will become top and the whole graph is incorrectly killed.

fix

61240a2

openjdk Bot added the rfr Pull request is ready for review label Jun 19, 2026

merykitty reviewed Jun 19, 2026

View reviewed changes

chhagedorn added 2 commits June 19, 2026 15:11

Review Quan-Anh

bf89360

fix tklass

b1de11c

merykitty reviewed Jun 19, 2026

View reviewed changes

improve checks and further refactor exclude_flat to make it easier to…

e3b4f88

… read

merykitty reviewed Jun 23, 2026

View reviewed changes

chhagedorn added 7 commits June 26, 2026 19:48

Update

0819467

Fix endless deopts with -XX:-UseArrayFlattening

2868844

Merge branch 'lworld' into JDK-8382226

da29a68

Update

02fdea6

Merge branch 'lworld' into JDK-8382226

30400c2

Improve comments

53493bc

Add test for void.class as well

20cd921

chhagedorn commented Jul 1, 2026

View reviewed changes

merykitty approved these changes Jul 1, 2026

View reviewed changes

openjdk Bot added the ready Pull request is ready to be integrated label Jul 1, 2026

merykitty reviewed Jul 1, 2026

View reviewed changes

	if (!t->is_klass()) {
	// a primitive Class (e.g., int.class) has null for a klass field
	return TypePtr::NULL_PTR;
	}

	bool exclude_flat = UseArrayFlattening && bs->array_copy_requires_gc_barriers(true, T_OBJECT, false, false, BarrierSetC2::Parsing) &&
	// Can src array be flat and contain oops?
	(orig_t == nullptr \|\| (!orig_t->is_not_flat() && (!orig_t->is_flat() \|\| orig_t->elem()->inline_klass()->contains_oops()))) &&
	// Can dest array be flat and contain oops?
	tklass->can_be_inline_array() && (!tklass->is_flat() \|\| tklass->is_aryklassptr()->elem()->is_instklassptr()->instance_klass()->as_inline_klass()->contains_oops());
	Node* not_objArray = exclude_flat ? generate_non_refArray_guard(klass_node, bailout) : generate_typeArray_guard(klass_node, bailout);

Uh oh!

Conversation

chhagedorn commented Jun 19, 2026 • edited by openjdk Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper Bot commented Jun 19, 2026

Uh oh!

openjdk Bot commented Jun 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mlbridge Bot commented Jun 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chhagedorn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

merykitty left a comment

Choose a reason for hiding this comment

Uh oh!

chhagedorn commented Jul 1, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

chhagedorn commented Jun 19, 2026 •

edited by openjdk Bot

Loading

openjdk Bot commented Jun 19, 2026 •

edited

Loading

mlbridge Bot commented Jun 19, 2026 •

edited

Loading